Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore(deps): upgrade to celestia-app v2.0.0-rc2 #3453

Draft
wants to merge 37 commits into
base: main
Choose a base branch
from

Conversation

rootulp
Copy link
Collaborator

@rootulp rootulp commented May 31, 2024

Update celestia-node to use celestia-app v2.0.0-rc2. A few changes that were needed:

  • celestia-app v1.x had a shares package. celestia-app v2.x uses the shares package from go-square.
  • celestia-app v1.x had a blob.types package with CreateCommitment. celestia-app v2.x uses CreateCommitment from the go-square inclusion package.
  • I had to update extended header verification to allow header.Version.App = 2. Added unit tests.
  • celestia-app v1.x had a lot of functionality included in the signer. celestia-app v2.x split a txClient from the signer. See: feat: add multi-account support celestia-app#3433
  • I had to update core_access.go a lot. Mostly inspired by feat: support multiple accounts #3451

Testing

I ran a script with: celestia-app v2.0.0-rc2 and configured it to upgrade at block height 3. celestia-node (built from this PR) continued to work:

2024-07-09T18:13:27.040-0400	INFO	header/store	store/store.go:367	new head	{"height": 2, "hash": "8776AEAF4114BD7E88E8DEC38445720D0BD857335BED99649957A43BB845EC87"}
2024-07-09T18:13:38.065-0400	INFO	header/store	store/store.go:367	new head	{"height": 3, "hash": "63D5C64521A964290BD21658314DDF60146AE419FE99026003048F74D2886B35"}
2024-07-09T18:13:49.093-0400	INFO	header/store	store/store.go:367	new head	{"height": 4, "hash": "FC7900918E716697A7CD6D9A4865B261F3E71D181F366E765AA53CF475223F9A"}

@rootulp rootulp self-assigned this May 31, 2024
@github-actions github-actions bot added the external Issues created by non node team members label May 31, 2024
@rootulp rootulp added the kind:deps Pull requests that update a dependency file label Jun 3, 2024
@rootulp

This comment was marked as outdated.

@renaynay renaynay added v0.15.0 Intended for v0.15.0 release kind:break! Attached to breaking PRs labels Jun 6, 2024
NamespaceId: namespace.ID(),
Data: data,
ShareVersion: uint32(shareVersion),
NamespaceVersion: uint32(namespace.Version()),
}

com, err := types.CreateCommitment(&blob)
com, err := inclusion.CreateCommitment(&blob, merkle.HashFromByteSlices, appconsts.SubtreeRootThreshold(v1.Version))
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[for reviewers] Invoking with v1.Version behaves identically to invoking with v2.Version because the implementation of this function doesn't actually care about the version that is provided as an argument.

See https://github.com/celestiaorg/celestia-app/blob/c0904490d3e60a8206b43728378b4ae4c097edd1/pkg/appconsts/versioned_consts.go#L21-L23

state/core_access.go Outdated Show resolved Hide resolved
@rootulp rootulp marked this pull request as ready for review June 6, 2024 20:37
blob/blobtest/testing.go:31:25: copylocks: call of append copies lock value: github.com/celestiaorg/go-square/blob.Blob contains google.golang.org/protobuf/internal/impl.MessageState contains sync.Mutex (govet)
                blobs = append(blobs, *appBlob)
@renaynay renaynay removed the v0.15.0 Intended for v0.15.0 release label Jun 25, 2024
@rootulp rootulp marked this pull request as ready for review June 27, 2024 17:51
@rootulp

This comment was marked as outdated.

@rootulp rootulp marked this pull request as draft July 2, 2024 19:19
@rootulp

This comment was marked as resolved.

@rootulp

This comment was marked as resolved.

@rootulp

This comment was marked as resolved.

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 9, 2024

TestTransfer fails because the core accessor returns a min gas price of "" which gets converted to 0. .002 is expected.

@rootulp
Copy link
Collaborator Author

rootulp commented Jul 10, 2024

  • I confirmed that the appConfig is getting overriden to supply a custom min gas price. This also repros if we use the default min gas price instead of overriding it.
  • It looks like the sdkCtx doesn't contain a populated minGasPrice here.
  • I can repro the test failing in celestia-app on v2.0.0-rc2.
  • I can't repro on celestia-app v1.12.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
external Issues created by non node team members kind:break! Attached to breaking PRs kind:deps Pull requests that update a dependency file v0.16.0 Intended for v0.16.0 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants